[tests] introduce VAETesterMixin to consolidate tests for slicing and tiling
#12374
Conversation
|
|
||
|
|
||
| class AutoencoderKLHunyuanVideoTests(ModelTesterMixin, UNetTesterMixin, unittest.TestCase): | ||
| class AutoencoderKLHunyuanVideoTests(ModelTesterMixin, VAETestMixin, UNetTesterMixin, unittest.TestCase): |
There was a problem hiding this comment.
Nit. Let's call it AutoencoderTesterMixin to be consistent. Also it looks like UNetTesterMixin is used in all the autoencoder tests? It only has a single test method. Let's add it to the AutoencoderTesterMixin and just have these classes inherit from their specific Mixin
|
I think we can remove the unittest dependency for these tests too? |
|
Oh I think for that, we need to make |
|
|
||
|
|
||
| class AutoencoderKLTests(ModelTesterMixin, UNetTesterMixin, unittest.TestCase): | ||
| class AsymmetricAutoencoderKLTests(ModelTesterMixin, unittest.TestCase): |
There was a problem hiding this comment.
The following autoencoder modules currently don't inherit from AutoencoderTesterMixin:
AsymmetricAutoencoderKLAutoencoderKLCosmosAutoencoderDCAutoencoderKLTemporalDecoderAutoencoderKLMagvitAutoencoderKLMochiAutoencoderOobleckVQModel
Is this because they don't have enable_tiling or enable_slicing methods? Since test_enable_disable_tiling and test_enable_disable_slicing check for these methods, it seems like it should be safe for them to inherit from AutoencoderTesterMixin. It would also have the added benefit of allowing them to have the one test that was moved from UNetTesterMixin into AutoencoderTesterMixin.
There was a problem hiding this comment.
It would also have the added benefit of allowing them to have the one test that was moved from UNetTesterMixin into AutoencoderTesterMixin.
Do you mean test_forward_with_norm_groups?
There was a problem hiding this comment.
Sorry, I edited my comment above to try to consolidate all of the models that don't inherit from AutoencoderTesterMixin. Do you think it would make sense for all autoencoder test classes to inherit from AutoencoderTesterMixin, as the slicing/tiling tests will be skipped if enable_slicing/enable_tiling methods are not available?
There was a problem hiding this comment.
Do you mean
test_forward_with_norm_groups?
Yeah I think that's the one
|
@dg845 thanks! Nice suggestion: #12374 (comment). I applied it accordingly. It also helped us reduce a bit more LoC. PTAL. |
|
I saw that in the https://github.com/huggingface/diffusers/actions/runs/18580604097/job/52974552004?pr=12374 CI run the following tests failed: I tried both of them locally on DGX and they passed (although my environment is not exactly synced up to the CI environment). Are these tests just flaky? It looks like the error might have to do with GPU availability. EDIT: It looks like these same tests failed again on the next CI run with the same error: https://github.com/huggingface/diffusers/actions/runs/18583169787/job/52981958899?pr=12374. |
|
Yeah those are unrelated to the PR, I think. |
|
Fixed the Mochi related tests: For LMK what you think. |
|
Also, regarding https://github.com/huggingface/diffusers/actions/runs/18583169787/job/52981958899?pr=12374, I think those are failing because of the CI hardware (uses T4 instances having compute capability of 7.5). Taking a look. The FP8 stuff there needs some hardware compatibility to work. |
3 participants
What does this PR do?
More reductions in LoC.